Well, I made some adjustments, mainly to how the toolchain is recognized.

The new idea is to have an optional parameter where users can provide their llvm_toolchain bin directory as input, and add that to PATH. (This is much better than the one I did before, but it does require more setup from users.)

Does this approach seem reasonable?
I do recognize that defining the llvm_toolchain for each target is not the most ergonomic, but it is the most flexible way. We have seen similar methods in aspect lint. This way, we even support custom LLVM rules.

I believe this would also allow users to de-prioritize the LLVM toolchain in the selection order of C++ toolchains(?).
I'm not sure.

format_test(
    name = "format_test",
    starlark = "@buildifier_prebuilt//:buildifier", # <- this line
	...
)

What still needs to be done:

• For a patch like this to land, we would need to change our testing. (The tests could have their own MODULE.bazel, an example is the llvm_toolchain repo) This is necessary since llvm_toolchain does not allow itself to be a transitive dependency (due to its size). -> This would fix foss tests, and external tests (these are the ones that fail)

To make it a bit more serious, I fixed the tests.
It does make the patch very messy :/

The highlights:

• I copied the testing solution of the llvm_toolchain, so:
  • Tests area separate bazel project, with its own MODULE.bazel. (I did have to modify every single test to accompany this :( )
• I modified the codechecker_pass target in legacy tests to show how users would use the llvm_toolchain. (This is just for demonstration)

The new idea is to have an optional parameter where users can provide their llvm_toolchain bin directory as input, and add that to PATH. (This is much better than the one I did before, but it does require more setup from users.)

Does this approach seem reasonable? I do recognize that defining the llvm_toolchain for each target is not the most ergonomic, but it is the most flexible way. We have seen similar methods in aspect lint. This way, we even support custom LLVM rules.

I believe this would also allow users to de-prioritize the LLVM toolchain in the selection order of C++ toolchains(?). I'm not sure.

Yes, this might be used. Basically this way we can provide path to particular binaries.
And I think this should be at least an option.
However, as I can understand, as soon as we use LLVM toolchain the whole package may want to use it for cc_binary, cc_libary etc. In some cases this is not the scenario users may want.

What still needs to be done:

• For a patch like this to land, we would need to change our testing. (The tests could have their own MODULE.bazel, an example is the llvm_toolchain repo) This is necessary since llvm_toolchain does not allow itself to be a transitive dependency (due to its size). -> This would fix foss tests, and external tests (these are the ones that fail)

I dont think creating more packages inside packages is a good idea. At least it is highly discouraged in Bazel.
Besides, what is the purpose?
To be honest I did not get "llvm_toolchain does not allow itself to be a transitive dependency" etc.
I think we need a meeting where you explain.

To make it a bit more serious, I fixed the tests. It does make the patch very messy :/

Well, the recent change made it impossible to review :)
I guess we should probably drop this...

The highlights:

• I copied the testing solution of the llvm_toolchain, so:

  • Tests area separate bazel project, with its own MODULE.bazel. (I did have to modify every single test to accompany this :( )

We should definitely avoid packages inside packages.
In general the idea of multiple packages is not good and often an indication of bad design.
I saw projects with similar mistakes - later it took much efforts to fix them.

• I modified the codechecker_pass target in legacy tests to show how users would use the llvm_toolchain. (This is just for demonstration)

Yes, I got the idea, Thanks!

Well, the recent change made it impossible to review :)

Sorry about that, I reverted those changes

To be honest I did not get "llvm_toolchain does not allow itself to be a transitive dependency" etc.

This is the offending line:
https://github.com/bazel-contrib/toolchains_llvm/blob/e15a648c23a6305c58f2cb54486ce99fe331696e/toolchain/extensions/llvm.bzl#L55

Not sure what an acceptable way to handle this is.
Lets discuss it in details in a meeting.

(I did make some miscellaneous changes, compared the issue of transitive dependencies it is not important)